@nimrod-starkware reviewed 10 files and all commit messages, and made 6 comments.
Reviewable status: 10 of 11 files reviewed, 6 unresolved discussions (waiting on @ArielElp and @yoavGrs).

crates/starknet_committer/src/db/index_db/create_index_tree_test.rs line 23 at r3 (raw file):

    fn compute_node_hash(_node_data: &NodeData<MockLeaf, HashOutput>) -> HashOutput {
        HashOutput(Felt::ZERO)
    }

This is needed for deserializing leaves right?

Code quote:

    fn compute_node_hash(_node_data: &NodeData<MockLeaf, HashOutput>) -> HashOutput {
        HashOutput(Felt::ZERO)
    }

crates/starknet_committer/src/db/index_db/mod.rs line 8 at r3 (raw file):

pub mod serde_tests;
#[cfg(test)]
pub mod test_utils;

only if there is "testing" feature to this crate

Suggestion:

#[cfg(any(feature = "testing", test))]
pub mod test_utils;

crates/starknet_committer/src/db/index_db/test_utils.rs line 40 at r3 (raw file):

// ================================================================================================
// Public API: Forest-level conversion
// ================================================================================================

delete plz

Code quote:

// ================================================================================================
// Public API: Forest-level conversion
// ================================================================================================

crates/starknet_committer/src/db/index_db/test_utils.rs line 64 at r3 (raw file):

// ================================================================================================
// Public API: Single trie conversion
// ================================================================================================

delete plz

Code quote:

// ================================================================================================
// Public API: Single trie conversion
// ================================================================================================

crates/starknet_committer/src/db/index_db/test_utils.rs line 85 at r3 (raw file):

// ================================================================================================
// Internal: Conversion logic
// ================================================================================================

delete

Code quote:

// ================================================================================================
// Internal: Conversion logic
// ================================================================================================

crates/starknet_committer/src/db/index_db/test_utils.rs line 95 at r3 (raw file):

    /// Panic on missing nodes - used for complete DB conversion where all nodes must exist.
    No,
}

Please use boolean instead

Code quote:

/// Controls behavior when a node is not found during conversion.
#[derive(Clone, Copy)]
enum SkipMissingNodes {
    /// Skip missing nodes silently - used for filled forest deltas where unmodified branches
    /// are not present.
    Yes,
    /// Panic on missing nodes - used for complete DB conversion where all nodes must exist.
    No,
}

@ArielElp made 6 comments.
Reviewable status: 7 of 11 files reviewed, 6 unresolved discussions (waiting on @nimrod-starkware and @yoavGrs).

crates/starknet_committer/src/db/index_db/create_index_tree_test.rs line 23 at r3 (raw file):

Yes :(

crates/starknet_committer/src/db/index_db/mod.rs line 8 at r3 (raw file):

Done (there is)

crates/starknet_committer/src/db/index_db/test_utils.rs line 40 at r3 (raw file):

Done.

crates/starknet_committer/src/db/index_db/test_utils.rs line 64 at r3 (raw file):

Done.

crates/starknet_committer/src/db/index_db/test_utils.rs line 85 at r3 (raw file):

Done.

crates/starknet_committer/src/db/index_db/test_utils.rs line 95 at r3 (raw file):

Done.

@nimrod-starkware reviewed 6 files and all commit messages, made 2 comments, and resolved 6 discussions.
Reviewable status: 10 of 11 files reviewed, 2 unresolved discussions (waiting on @ArielElp, @dorimedini-starkware, and @yoavGrs).

crates/starknet_committer/src/db/index_db/test_utils.rs line 154 at r4 (raw file):

    index_layout_storage: &mut MapStorage,
    current_index: NodeIndex,
    current_hash: HashOutput,

Please try to use FactsSubTree to gather these two.

Code quote:

    current_index: NodeIndex,
    current_hash: HashOutput,

crates/starknet_committer/src/db/index_db/test_utils.rs line 171 at r4 (raw file):

    } else {
        PatriciaPrefix::InnerNode.into()
    };

subtree.get_root_db_key

Code quote:

    let db_prefix = if current_index.is_leaf() {
        FactsLeaf::get_static_prefix(key_context)
    } else {
        PatriciaPrefix::InnerNode.into()
    };

@ArielElp made 2 comments.
Reviewable status: 7 of 11 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware, @nimrod-starkware, and @yoavGrs).

crates/starknet_committer/src/db/index_db/test_utils.rs line 154 at r4 (raw file):

Done.

crates/starknet_committer/src/db/index_db/test_utils.rs line 171 at r4 (raw file):

Done.

@nimrod-starkware reviewed 5 files and all commit messages, made 9 comments, and resolved 2 discussions.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @ArielElp, @dorimedini-starkware, and @yoavGrs).

crates/starknet_committer/src/db/index_db/mod.rs line 8 at r3 (raw file):

why did you revert?

crates/starknet_committer/src/db/index_db/test_utils.rs line 42 at r6 (raw file):

    skip_missing: bool,
) -> MapStorage {
    let mut contract_leaves: Option<Vec<(NodeIndex, ContractState)>> = Some(Vec::new());

Suggestion:

let mut contract_leaves: Vec<(NodeIndex, ContractState)>> = Vec::new();

crates/starknet_committer/src/db/index_db/test_utils.rs line 48 at r6 (raw file):

            roots.contracts_trie_root_hash,
            &EmptyKeyContext,
            &mut contract_leaves,

Suggestion:

&mut Some(contract_leaves),

crates/starknet_committer/src/db/index_db/test_utils.rs line 54 at r6 (raw file):

        .0;

    for (index, contract_leaf) in contract_leaves.unwrap() {

Suggestion:

for (index, contract_leaf) in contract_leaves {

crates/starknet_committer/src/db/index_db/test_utils.rs line 76 at r6 (raw file):

        )
        .await;
    index_storage.extend(classes_storage.0);

Suggestion:

    let mut index_storage =
        convert_single_trie::<FactsLayout::ContractStateDbLeaf, IndexNodeLayout::ContractStateDbLeaf, EmptyKeyContext>(
            storage,
            roots.contracts_trie_root_hash,
            &EmptyKeyContext,
            &mut contract_leaves,
            skip_missing,
        )
        .await
        .0;

    for (index, contract_leaf) in contract_leaves.unwrap() {
        let contract_address = try_node_index_into_contract_address(&index).unwrap();
        let storage_root = contract_leaf.storage_root_hash;
        let contract_storage_entries =
            convert_single_trie::<
                also here.....
                ContractAddress,
            >(storage, storage_root, &contract_address, &mut None, skip_missing)
            .await;
        index_storage.extend(contract_storage_entries.0);
    }

    let classes_storage =
        convert_single_trie::<and here, EmptyKeyContext>(
            storage,
            roots.classes_trie_root_hash,
            &EmptyKeyContext,
            &mut None,
            skip_missing,
        )
        .await;
    index_storage.extend(classes_storage.0);

crates/starknet_committer/src/db/index_db/test_utils.rs line 96 at r6 (raw file):

{
    convert_single_trie(storage, root_hash, key_context, current_leaves, false).await
}

IMO this wrapper is not needed (I don't think you even use it)

Code quote:

/// Converts a single Facts-layout trie to Index-layout.
/// Expects all nodes to exist (panics if a node is missing).
pub async fn convert_facts_db_to_index_db<FactsLeaf, IndexLeaf, KeyContext>(
    storage: &mut MapStorage,
    root_hash: HashOutput,
    key_context: &KeyContext,
    current_leaves: &mut Option<Vec<(NodeIndex, FactsLeaf)>>,
) -> MapStorage
where
    FactsLeaf: Leaf + Into<IndexLeaf> + HasStaticPrefix<KeyContext = KeyContext>,
    IndexLeaf: Leaf + HasStaticPrefix<KeyContext = KeyContext>,
    TreeHashFunctionImpl: TreeHashFunction<IndexLeaf>,
    KeyContext: Sync,
{
    convert_single_trie(storage, root_hash, key_context, current_leaves, false).await
}

crates/starknet_committer/src/db/index_db/test_utils.rs line 136 at r6 (raw file):

    key_context: &KeyContext,
    current_leaves: &mut Option<Vec<(NodeIndex, FactsLeaf)>>,
    skip_missing: bool,

IMO should renamed to indicate the opposite

Suggestion:

panic_on_missing_node: bool,

crates/starknet_committer/src/db/index_db/test_utils.rs line 163 at r6 (raw file):

            );
        }
    };

see above

Suggestion:

        if panic_on_missing_node {
            panic!(
                "Node not found in storage: index={:?}, hash={:?}. If converting a filled forest, \
                 use convert_facts_filled_forest_to_index.",
                subtree.root_index, subtree.root_hash
            );
        } else {
            return;
        }
    };

crates/starknet_committer/src/db/index_db/test_utils.rs line 241 at r6 (raw file):

        INDEX_LAYOUT_DB_KEY_SEPARATOR,
        &subtree.root_index.0.to_be_bytes(),
    );

Please use IndexLayoutSubTree::get_root_db_key here

Code quote:

    let index_db_key = create_db_key(
        IndexLeaf::get_static_prefix(key_context),
        INDEX_LAYOUT_DB_KEY_SEPARATOR,
        &subtree.root_index.0.to_be_bytes(),
    );

@ArielElp made 8 comments.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @dorimedini-starkware, @nimrod-starkware, and @yoavGrs).

crates/starknet_committer/src/db/index_db/test_utils.rs line 96 at r6 (raw file):

I am using it in create_index_tree_test.rs

In the next PR I'm also using convert_facts_forest_db_to_index_db for the test that uses real leaf types.

crates/starknet_committer/src/db/index_db/test_utils.rs line 136 at r6 (raw file):

Done.

crates/starknet_committer/src/db/index_db/test_utils.rs line 163 at r6 (raw file):

Done.

crates/starknet_committer/src/db/index_db/test_utils.rs line 241 at r6 (raw file):

Done.

crates/starknet_committer/src/db/index_db/test_utils.rs line 42 at r6 (raw file):

    skip_missing: bool,
) -> MapStorage {
    let mut contract_leaves: Option<Vec<(NodeIndex, ContractState)>> = Some(Vec::new());

Done (this requires changing the signature to &mut Option<&mut Vec> to allow reusing contract_leaves for multiple recursive calls)

crates/starknet_committer/src/db/index_db/test_utils.rs line 48 at r6 (raw file):

            roots.contracts_trie_root_hash,
            &EmptyKeyContext,
            &mut contract_leaves,

Done

crates/starknet_committer/src/db/index_db/test_utils.rs line 54 at r6 (raw file):

        .0;

    for (index, contract_leaf) in contract_leaves.unwrap() {

Done.

crates/starknet_committer/src/db/index_db/test_utils.rs line 76 at r6 (raw file):

        )
        .await;
    index_storage.extend(classes_storage.0);

Done.

@nimrod-starkware reviewed 1 file and all commit messages, made 1 comment, and resolved 8 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dorimedini-starkware and @yoavGrs).

@dorimedini-starkware reviewed 5 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ArielElp and @yoavGrs).

crates/starknet_committer/src/db/index_db/test_utils.rs line 203 at r7 (raw file):

                panic_on_missing_node,
            )
            .await;

are the tests fast?
if not, may be worth gathering these in a tokio::JoinSet

Code quote:

            traverse_and_convert::<FactsLeaf, IndexLeaf, KeyContext>(
                facts_storage,
                index_layout_storage,
                left_subtree,
                key_context,
                current_leaves,
                panic_on_missing_node,
            )
            .await;
            traverse_and_convert::<FactsLeaf, IndexLeaf, KeyContext>(
                facts_storage,
                index_layout_storage,
                right_subtree,
                key_context,
                current_leaves,
                panic_on_missing_node,
            )
            .await;

crates/starknet_committer/src/db/index_db/test_utils.rs line 247 at r7 (raw file):

    let index_db_key =
        IndexLayoutSubTree::create(SortedLeafIndices::default(), subtree.root_index, EmptyNodeData)
            .get_root_db_key::<IndexLeaf>(key_context);

you don't really need to create a subtree to get the root DB key, right? maybe you can refactor a bit to be able to do this statically?

Code quote:

        IndexLayoutSubTree::create(SortedLeafIndices::default(), subtree.root_index, EmptyNodeData)
            .get_root_db_key::<IndexLeaf>(key_context);

@ArielElp made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware and @yoavGrs).

crates/starknet_committer/src/db/index_db/test_utils.rs line 203 at r7 (raw file):

Yes, we're only using the conversion on small unitests trees (or height 251 but very sparse).

crates/starknet_committer/src/db/index_db/test_utils.rs line 247 at r7 (raw file):

Originally I didn't use subtrees but changed following Nimrod's review. I think it's better, since before, all the logic of get_root_db_key was inlined.

@dorimedini-starkware made 1 comment and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArielElp and @yoavGrs).

crates/starknet_committer/src/db/index_db/test_utils.rs line 247 at r7 (raw file):

true, this is better than inlining; I'm just wondering if some internal static logic can't be split out and reused. non-blocking anyway

@dorimedini-starkware reviewed 2 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @yoavGrs).